Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tests: add rule type check for flowbits v8 #1532

Closed
wants to merge 2 commits into from
Closed

tests: add rule type check for flowbits v8 #1532

wants to merge 2 commits into from

Conversation

hadiqaalamdar
Copy link
Contributor

@hadiqaalamdar hadiqaalamdar commented Dec 11, 2023 •
edited
Loading

Task #6309

Redmine ticket: https://redmine.openinfosecfoundation.org/issues/6309
Previous PR: #1529

Suricata PR: OISF/suricata#10026

Output from console:

===> rules/flowbits: OK

PASSED:  3
FAILED:  0
SKIPPED: 6

Output from stdout file:

Notice: suricata: This is Suricata version 8.0.0-dev (9a2432696 2023-12-11) running in USER mode [LogVersion:suricata.c:1147]
Warning: threshold-config: Error opening file: "/usr/local/etc/suricata//threshold.config": Permission denied [SCThresholdConfInitContext:util-threshold-config.c:177]
Warning: detect-flowbits: flowbit 'fb3' is checked but not set. Checked in 4 and 1 other sigs [DetectFlowbitsAnalyze:detect-flowbits.c:596]
Warning: detect-flowbits: flowbit 'fb4' is checked but not set. Checked in 5 and 0 other sigs [DetectFlowbitsAnalyze:detect-flowbits.c:596]
Warning: detect-flowbits: flowbit 'fb5' is checked but not set. Checked in 12 and 0 other sigs [DetectFlowbitsAnalyze:detect-flowbits.c:596]

This was referenced Dec 11, 2023
Closed
Closed
inashivb
inashivb requested changes Dec 12, 2023
View reviewed changes
Copy link
Member

@inashivb inashivb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is missing @jufajardini 's review update from the last PR. ref: #1529 (comment)

Please fix that. From the first look, it's looking good otherwise :)

@hadiqaalamdar
Copy link
Contributor Author

I believe this is missing @jufajardini 's review update from the last PR. ref: #1529 (comment)

Please fix that. From the first look, it's looking good otherwise :)

Sorry, I just saw the comment. I was wondering if we need a rule for noalert now if we've already removed that field from detect-flowbits files?

@jufajardini
Copy link
Contributor

I believe this is missing @jufajardini 's review update from the last PR. ref: #1529 (comment)
Please fix that. From the first look, it's looking good otherwise :)

Sorry, I just saw the comment. I was wondering if we need a rule for noalert now if we've already removed that field from detect-flowbits files?

I think we could keep it. My reasoning for this is that with this engine-analysis work we are also documenting and showcasing things, and therefore there is value in showing what will it look like if you use flowbits:noalert.

Adding the check to the test.yaml file will help show where to find information about that, too.

@hadiqaalamdar
Copy link
Contributor Author

I think we could keep it. My reasoning for this is that with this engine-analysis work we are also documenting and showcasing things, and therefore there is value in showing what will it look like if you use flowbits:noalert.

Adding the check to the test.yaml file will help show where to find information about that, too.

Okay, I'm not sure how to alter the yaml file such that the noalert rule doesn't create an error.

@jufajardini
Copy link
Contributor

I think we could keep it. My reasoning for this is that with this engine-analysis work we are also documenting and showcasing things, and therefore there is value in showing what will it look like if you use flowbits:noalert.
Adding the check to the test.yaml file will help show where to find information about that, too.

Okay, I'm not sure how to alter the yaml file such that the noalert rule doesn't create an error.

That's ok, we're figuring this one out as we progress :)

From the eve log from the previous PR (#1529 (comment)), I think that we can:
i) have a rule that actually sets a flowbit, but also calls flowbit:noalert (just to ensure we're using a rule that makes some sense :P
ii) besides the check for the set flowbit, we can have something like:

flag[4]: "noalert"

I've tried a local test, and this seems to work.
What do you think?

@hadiqaalamdar hadiqaalamdar mentioned this pull request Dec 13, 2023
Closed
@hadiqaalamdar
Copy link
Contributor Author

New PR: #1542

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inashivb inashivb inashivb requested changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hadiqaalamdar @jufajardini @inashivb